Refactor zend_object_handlers API to pass zend_object* and zend_string* insted of zval(s). by dstogov · Pull Request #3780 · php/php-src

dstogov · 2019-01-31T15:51:05Z

No description provided.

nikic · 2019-01-31T16:07:32Z

Looks like Windows build is broken:

ext\com_dotnet\com_handlers.c(471): error C2039: 'value': is not a member of '_zend_object'
c:\projects\php-src\zend\zend_types.h(370): note: see declaration of '_zend_object'

dstogov · 2019-01-31T16:41:47Z

On 1/31/19 7:08 PM, Nikita Popov wrote: Looks like Windows build is broken: ext\com_dotnet\com_handlers.c(471): error C2039: 'value': is not a member of '_zend_object' c:\projects\php-src\zend\zend_types.h(370): note: see declaration of '_zend_object'

Very possible. I don't run Windows. I'll try to fix this blindly or ask Anatol to help. Thanks. Dmitry.

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3780 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACZM0pkRDlvue5u8NcLk8yvHCYQPh3m9ks5vIxTvgaJpZM4acmDC>.

krakjoe · 2019-02-01T05:10:20Z

This is nice for extension authors too, I like ...

@weltling @KalleZ can we get some windows input please ?

dstogov · 2019-02-01T07:04:41Z

This patch was especially delayed for PHP-8, where source compatibility is expected to be broken.

…

________________________________ From: Joe Watkins <notifications@github.com> Sent: Friday, February 1, 2019 8:10:55 AM To: php/php-src Cc: Dmitry Stogov; Author Subject: Re: [php/php-src] Refactor zend_object_handlers API to pass zend_object* and zend_string* insted of zval(s). (#3780) This is nice for extension authors too, I like ... @weltling<https://github.com/weltling> @KalleZ<https://github.com/KalleZ> can we get some windows input please ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#3780 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACZM0sJDlnfPtbKUne24rYR3c1nMrncPks5vI8xfgaJpZM4acmDC>.

krakjoe · 2019-02-01T07:08:27Z

@dstogov ack, added 8 milestone to make that explicit ... although seems obvious :)

nikic

LGTM, apart from one get_properties usage.

nikic · 2019-02-01T08:30:20Z

Not sure, but we might want to change zend_read_property to accept a zobj too?

May be make sense, but together with zend_update_property...(), zend_unset_property...(), some exception functions and may be others. Let do it separately.

dstogov · 2019-02-01T16:06:53Z

@nikic thanks for review. I've fixed the get_properties() issue.

cmb69 · 2019-02-01T18:18:52Z

I've made a patch for ext/com_dotnet. It compiles, and the test suite shows no regression. Obviously, this part can't work, though.

weltling · 2019-02-01T21:17:39Z

@cmb69 ah, you beat me to it, thanks :)

With the non working part - you can check whether you allocate/free that zval. Or, change the php_com_saproxy->obj so it takes zend_object *. The second option were probably better.

Thanks.

weltling · 2019-02-01T21:24:31Z

@cmb69 also it were better to just adjust the CDNO_FETCH* macros, not putting the exact code everywhere, to make the diff smaller and keep the API.

Thanks.

cmb69 · 2019-02-01T23:01:56Z

Thanks, @weltling!

Or, change the php_com_saproxy->obj so it takes zend_object *.

That's intriguing! php_com_saproxy->zobj is a zval *, and php_com_saproxy->obj is a php_com_dotnet_object *. Since the first member of php_com_dotnet_object is the very same zend_object that php_com_saproxy->zobj refers to, it seems we could drop php_com_saproxy->zobj altogether, since PHP 7 moved the refcounting to the zend_object (which is apparently the only reason why a zval * is stored).

also it were better to just adjust the CDNO_FETCH* macros,

The original CDNO_FETCH() macro is still needed elsewhere, so we'd have to introduce another macro.

Anyhow, I'll have a closer look tomorrow.

cmb69 · 2019-02-01T23:07:48Z

Since the first member of php_com_dotnet_object is the very same zend_object […]

Which looks not right for PHP 7, though.

PS: Seems to be okay in this case, since there are no default properties.

…g* insted of zval(s).

dstogov · 2019-02-04T08:51:10Z

Thanks. I'll merge this today.

…

________________________________ From: Christoph M. Becker <notifications@github.com> Sent: Friday, February 1, 2019 9:19:29 PM To: php/php-src Cc: Dmitry Stogov; Mention Subject: Re: [php/php-src] Refactor zend_object_handlers API to pass zend_object* and zend_string* insted of zval(s). (#3780) I've made a patch for ext/com_dotnet<https://gist.github.com/cmb69/6cc9ff6883b2d35ae6ce1ac05b6559c3>. It compiles, and the test suite shows no regression. Obviously, this part<https://gist.github.com/cmb69/6cc9ff6883b2d35ae6ce1ac05b6559c3#file-patch-L336-L340> can't work, though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#3780 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACZM0qIs4etDHH3A27nNqunBbebIuh8mks5vJIUxgaJpZM4acmDC>.

krakjoe · 2019-02-04T08:57:18Z

@dstogov are @cmb69 and @weltling finished ? it sounds like they're not ...

dstogov · 2019-02-04T09:10:19Z

@cmb69 @weltling @krakjoe I hope, I fixed the php_com_saproxy->zobj issue and going to merge after testing.

nikic · 2019-02-04T10:23:30Z

Can you please also add some notes about this to UPGRADING.INTERNALS?

dstogov · 2019-02-04T10:28:15Z

@nikic OK I'll do. The PR is merged.

cmb69 · 2019-02-04T13:58:14Z

Thanks, @dstogov! I've pushed a minor fix (a1608b4); looks good to me now. (Besides the aborting bug39606.phpt, but that is unrelated to this PR; will try to have a look.)

dstogov · 2019-02-04T14:54:05Z

@cmb69 Thank you :)

krakjoe added the Feature label Feb 1, 2019

krakjoe added this to the PHP 8.0 milestone Feb 1, 2019

nikic approved these changes Feb 1, 2019

View reviewed changes

dstogov added 3 commits February 4, 2019 11:47

Refactor zend_object_handlers API to pass zend_object* and zend_strin…

ae4a6a5

…g* insted of zval(s).

Reuse cache_slot

75782a2

More accurate get_properties() usage.

85790c5

ext/com_dotnet support for new object handlers API

f732cf6

dstogov force-pushed the obj_handlers branch from a462f4d to f732cf6 Compare February 4, 2019 09:08

dstogov closed this Feb 4, 2019

Uh oh!

Conversation

dstogov commented Jan 31, 2019

Uh oh!

nikic commented Jan 31, 2019

Uh oh!

dstogov commented Jan 31, 2019 via email

Uh oh!

krakjoe commented Feb 1, 2019

Uh oh!

dstogov commented Feb 1, 2019 via email

Uh oh!

krakjoe commented Feb 1, 2019

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

dstogov Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

dstogov commented Feb 1, 2019

Uh oh!

cmb69 commented Feb 1, 2019

Uh oh!

weltling commented Feb 1, 2019

Uh oh!

weltling commented Feb 1, 2019

Uh oh!

cmb69 commented Feb 1, 2019

Uh oh!

cmb69 commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstogov commented Feb 4, 2019 via email

Uh oh!

krakjoe commented Feb 4, 2019

Uh oh!

dstogov commented Feb 4, 2019

Uh oh!

nikic commented Feb 4, 2019

Uh oh!

dstogov commented Feb 4, 2019

Uh oh!

cmb69 commented Feb 4, 2019

Uh oh!

dstogov commented Feb 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cmb69 commented Feb 1, 2019 •

edited

Loading